Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refac js components #166

Merged
merged 12 commits into from
Feb 6, 2024
Merged

Refac js components #166

merged 12 commits into from
Feb 6, 2024

Conversation

jkulhanek
Copy link
Contributor

This PR implements the following:

  • Split components into individual tsx files
  • Stores attributes and values inside components' configs
  • Renames initial_value->value for add_gui_* methods

The goal is gradually moving towards a unified API to add new components.

@brentyi - please let me know what you think.

@jkulhanek jkulhanek requested a review from brentyi January 29, 2024 14:09
@jkulhanek jkulhanek force-pushed the jkulhanek/refac-js-components branch 3 times, most recently from 3b162ae to 5a49853 Compare January 29, 2024 15:21
@jkulhanek
Copy link
Contributor Author

@brentyi , if you like it, can you please help me with the mypy error? My limited python knowledge is not enough to solve the issue :/

@brentyi
Copy link
Collaborator

brentyi commented Jan 29, 2024

Need to find time to go through this in-depth, but looks good from a skim! Also happy to look into the mypy error...

The main thing I'm uneasy about is the initial_value= => value= API change in the add_gui_*() methods: is this about consistency with other properties / attributes? Would it make a refactor a la #161 much easier down-the-line? Going through with this would mean either an eventual breaking change or indefinitely supporting both names, both of which would be nice to avoid.

@jkulhanek
Copy link
Contributor Author

The rename is both for consistency and to make #161 easier. If we decide to use code generation for #161 then it wouldn’t matter much. For pure python typing this would help. However, I understand that introducing a breaking change may not be preferable for you. I can undo the name change if you want?

@brentyi
Copy link
Collaborator

brentyi commented Jan 30, 2024

Okay, let's switch it back. Reasoning: it seems unlikely we'll converge to a ParamSpec-based refactor we're happy with, and value is a little bit special (it can be updated by the client) so I'm okay with the inconsistency.

@jkulhanek
Copy link
Contributor Author

Okay, let's switch it back. Reasoning: it seems unlikely we'll converge to a ParamSpec-based refactor we're happy with, and value is a little bit special (it can be updated by the client) so I'm okay with the inconsistency.

Why is value special? You can change other properties as well: visible, disables, etc., no? I was also thinking about the following use case: in future perhaps we want to enable property binding to make the app faster (bind a property to another component's property).

@jkulhanek jkulhanek force-pushed the jkulhanek/refac-js-components branch from 5a49853 to 2d6802f Compare January 30, 2024 10:52
@jkulhanek
Copy link
Contributor Author

I undid the api change.

@jkulhanek jkulhanek force-pushed the jkulhanek/refac-js-components branch from 2d6802f to 6909002 Compare January 30, 2024 11:00
@jkulhanek
Copy link
Contributor Author

But, initial_value->value is still done in messages! Is that ok?

@jkulhanek jkulhanek force-pushed the jkulhanek/refac-js-components branch from 6909002 to c7b0b0c Compare January 30, 2024 11:10
@jkulhanek jkulhanek force-pushed the jkulhanek/refac-js-components branch from c7b0b0c to 9e5a015 Compare January 30, 2024 12:05
@brentyi
Copy link
Collaborator

brentyi commented Jan 30, 2024

Why is value special? You can change other properties as well: visible, disables, etc., no?

The difference I was alluding to is that everything in a GuiAdd* message (disabled, hint, initial_value, etc) can only be changed via an assignment from the server. Whereas the value can also be updated by the client.

For the internals and in messages, there are minor but important semantic differences between what it means when we write value and when we write initial_value: when we send a GuiAdd[...] message to update just a single property, for example for dropdowns:

@options.setter
def options(self, options: Iterable[StringType]) -> None:
self._impl_options = tuple(options)
if self._impl.initial_value not in self._impl_options:
self._impl.initial_value = self._impl_options[0]
self._impl.gui_api._get_api()._queue(
GuiAddDropdownMessage(
order=self._impl.order,
id=self._impl.id,
label=self._impl.label,
container_id=self._impl.container_id,
hint=self._impl.hint,
initial_value=self._impl.initial_value,
options=self._impl_options,
)
)
if self.value not in self._impl_options:
self.value = self._impl_options[0]

we re-send the original initial_value, but this won't overwrite whatever the client has stored if it updates the value of the input. This was convenient at the time because it lets the server update input properties without worrying about things like race conditions. Does this make sense?

@jkulhanek
Copy link
Contributor Author

I understand the motivation. I still think doing partial updates (updating only changed properties) would have been safer and more principled way to do it. In the case where you set disabled/visible attributes from the client to server, they are not synchronised from server to client, right? Wouldn't it be better to treat all props the same?

@brentyi
Copy link
Collaborator

brentyi commented Jan 31, 2024

I still think doing partial updates (updating only changed properties) would have been safer and more principled way to do it.

Yeah, I agree. I just didn't bother with the initial implementation because sending the full list of properties was much simpler to implement.

Did you have an implementation in mind for doing a partial update? It seems like we could do one of:

  • Reuse the same message type, but make all of the properties optional so we only need to send what's needed? Use a TypedDict with total=false?
  • (in the .options example above) Add a new SetGuiDropdownOptions message?
  • Use a generic SetGuiProp message?

I think the internal initial_value => value name change makes sense if we go with one of these options. I'd be slightly opposed to naming a field value that's actually functioning as an initial_value (eg not overwriting the client-side value when sent from the server).

In the case where you set disabled/visible attributes from the client to server, they are not synchronised from server to client, right?

That's correct, although I was never planning on adding mechanics to set these attributes from the client. If we added some kind of property binding like you mentioned this might become possible, although I'm skeptical the complexity tradeoff here.

@jkulhanek
Copy link
Contributor Author

I think SetGuiProp is the easiest to implement, although a bit less safe on the python side. However, if eventually the client handles are generic, there should be no risk of introducing bugs this way. The benefit is that the current messages could stay the same.
I believe adding options wouldn't scale that well. Perhaps a generic approach is preferable?

Which one would you prefer?

@jkulhanek
Copy link
Contributor Author

Or perhaps we can have typeddict props (ButtonProps, TextInputProps) and AddGuiComponent, UpdateGuiComponent messages?

@jkulhanek
Copy link
Contributor Author

Please let me know what you think now.

@brentyi
Copy link
Collaborator

brentyi commented Feb 6, 2024

(edit: now fixed) getting closer, need to fix some performance regressions

in this branch:

Screen.Recording.2024-02-06.at.1.59.12.AM.mov

on main:

Screen.Recording.2024-02-06.at.2.00.31.AM.mov

@brentyi brentyi enabled auto-merge (squash) February 6, 2024 20:01
@brentyi brentyi merged commit 0d05686 into main Feb 6, 2024
12 checks passed
@brentyi brentyi deleted the jkulhanek/refac-js-components branch February 6, 2024 20:03
@brentyi brentyi mentioned this pull request Feb 29, 2024
@brentyi brentyi mentioned this pull request Sep 24, 2024
yzslab pushed a commit to yzslab/viser that referenced this pull request Oct 20, 2024
* Refac components

* GUI api using partial update messages

* Update message typecasting

* Embrace untyped GuiUpdateMessage?

* ruff

* Address CI errors

* Fix tab groups, spacing tweaks

* Fix performance regression (components were unnecessarily re-rendering)

* ruff

* move multislider source

---------

Co-authored-by: Brent Yi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants